Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that a module within a namespace package can be found by --pyargs #1597

Merged
merged 3 commits into from
Jun 19, 2016

Conversation

taschini
Copy link
Contributor

@taschini taschini commented Jun 8, 2016

This PR supersedes #1568 and addresses the problem highlighted in #1567 and in part #478.

See #1568 for the description of the test setup.

Unlike #1568, this PR does not attempt to emulate the import machinery to determine the filename of a package or directory, delegating instead most of the work to the pkgutil module of the standard library.

NB In case of a non-top-level module or package, pkgutil.find_loader will import the parent package or packages.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.246% when pulling 2481b7a on taschini:pyargs-fix into 70fdab4 on pytest-dev:master.

return x
try:
path = loader.get_filename()
except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is usually better to avoid bare except clauses like this... could this be replaced by except ImportError?

taschini added a commit to taschini/pytest that referenced this pull request Jun 9, 2016
@taschini
Copy link
Contributor Author

taschini commented Jun 9, 2016

Appveyor caught a little problem with a test setup using PyPy on Windows: I'll fix that.

@nicoddemus I'll incorporate your feedback. The reason why that try-except clause is needed is that sometimes _parsearg() gets invoked with AssertionRewritingHook, which does not define a get_filename method, already in place. Hence, the exception you'll get is an AttributeError.

I'll catch explicitly that exception and add a comment to explain why that is necessary.

taschini added a commit to taschini/pytest that referenced this pull request Jun 13, 2016
Fixed problem caused in a test on Windows by file left open by PyPy and not immediately garbage collected.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 92.21% when pulling 5403f27 on taschini:pyargs-fix into 70fdab4 on pytest-dev:master.

@taschini
Copy link
Contributor Author

I incorporated the feedback from @nicoddemus and also fixed a problem caused in a test on Windows by a file left open by PyPy and not immediately garbage collected.

@nicoddemus
Copy link
Member

Thanks! 😁

LGTM. Could someone else take a look as well?

Also, we need a CHANGELOG entry and add @taschini to AUTHORS before merging.

@taschini
Copy link
Contributor Author

Shall I add the entries in CHANGELOG and AUTHORS or is it something that typically you guys do?

@nicoddemus
Copy link
Member

Please do. We usually ask for PR submitters to do it, but this has gone back and forth a few times already and I didn't want to ask for one more, hehehe. 😅

taschini added a commit to taschini/pytest that referenced this pull request Jun 13, 2016
@nicoddemus
Copy link
Member

It seems this is now in conflict. Would you please merge/rebase to fix it? Thanks!

@taschini
Copy link
Contributor Author

Rebased the PR.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 92.21% when pulling 1218392 on taschini:pyargs-fix into 66e66f6 on pytest-dev:master.

@nicoddemus
Copy link
Member

Thanks! 😁

I will merge this later unless someone says otherwise.

@nicoddemus nicoddemus merged commit d81ee9a into pytest-dev:master Jun 19, 2016
@nicoddemus
Copy link
Member

Thanks, sorry for the delay! 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants